Scala 2.12 upgrade preview with mysql tests fixed #346

pankajroark · 2017-05-07T14:55:59Z

Builds on top of #342 fixing the mysql tests that the scalacheck upgrade breaks.

1. Incremented all required versions to value that included scala 2.12 support. See diff for details. 2. Modified RedisStore to use Buf instead of ChannelBuffer due to changes in underlying client. To ease this transition a method to convert Store[Buf, Buf] to Store[ChannelBuffer, ChannelBuffer] is provided on RedisStore. 3. Removed support for Scala 2.10. Two libraries lacked support for scala 2.10 and 2.12 on the same version: twiiter-util and finagle. Supporting 2.10 finagle and 2.12 finagle was not possible due to interface changes. 4. Various packages changed and were fixed. 5. Added target to travis.yml and build.sbt to include scala 2.12. 6. Set source and build target to java 8 to support scala 2.12.

pankajroark · 2017-05-07T15:45:21Z

All tests have passed afaict, looks like travis is having issues.

ttim

Looks good overall, the only questionable thing for me - should we enable 2.12 as part of this change?

ttim · 2017-05-08T20:18:28Z

.travis.yml

    - scala: 2.11.8
+      jdk: oraclejdk8


Should we do this for 2.11?

No, I guess in the original PR Ben may have added it for consistency. Let me remove it.

ttim · 2017-05-08T20:19:01Z

build.sbt

-  crossScalaVersions := Seq("2.10.6", "2.11.7"),
-  javacOptions ++= Seq("-source", "1.6", "-target", "1.6"),
-  javacOptions in doc := Seq("-source", "1.6"),
+  crossScalaVersions := Seq("2.11.7", "2.12.1"),


If we add 2.12 here then should we also enable travis for 2.12?

can we use 2.11.11 and 2.12.2?

ttim · 2017-05-08T20:20:10Z

...ra/src/test/scala/com/twitter/storehaus/algebra/query/CalendarTimeQueryRangeProperties.scala

@@ -19,7 +19,8 @@ package com.twitter.storehaus.algebra.query
 import org.scalacheck.{ Gen, Arbitrary }
 import org.scalacheck.Prop._
 import org.scalacheck.Properties
-import com.twitter.scalding._
+import com.twitter.scalding.{RichDate, DateRange, Duration, AbsoluteDuration, Years,


Do we need it in this review?

+1 to minimize. I prefer to keep the diffs as minimal as possible (and personally never mix style and logic changes if possible).

johnynek · 2017-05-08T20:34:07Z

.travis.yml

      script: umask 0022 && ./sbt ++$TRAVIS_SCALA_VERSION clean coverage test coverageReport mimaReportBinaryIssues
      after_success:
        - bash <(curl -s https://codecov.io/bash)
+#    - scala: 2.12.1


what's the story here?

Ahh... this got carried over from Ben's original PR. Let me uncomment this.

johnynek · 2017-05-08T20:34:27Z

build.sbt

-  crossScalaVersions := Seq("2.10.6", "2.11.7"),
-  javacOptions ++= Seq("-source", "1.6", "-target", "1.6"),
-  javacOptions in doc := Seq("-source", "1.6"),
+  crossScalaVersions := Seq("2.11.7", "2.12.1"),


can we use 2.11.11 and 2.12.2?

johnynek · 2017-05-08T20:35:31Z

...ra/src/test/scala/com/twitter/storehaus/algebra/query/CalendarTimeQueryRangeProperties.scala

@@ -19,7 +19,8 @@ package com.twitter.storehaus.algebra.query
 import org.scalacheck.{ Gen, Arbitrary }
 import org.scalacheck.Prop._
 import org.scalacheck.Properties
-import com.twitter.scalding._
+import com.twitter.scalding.{RichDate, DateRange, Duration, AbsoluteDuration, Years,


+1 to minimize. I prefer to keep the diffs as minimal as possible (and personally never mix style and logic changes if possible).

johnynek · 2017-05-08T20:36:00Z

storehaus-memcache/src/main/scala/com/twitter/storehaus/memcache/MemcacheStore.scala

@@ -70,7 +70,6 @@ object MemcacheStore {
      .tcpConnectTimeout(timeout)
      .requestTimeout(timeout)
      .connectTimeout(timeout)
-      .readerIdleTimeout(timeout)


this seems like a maybe logical change. Are we sure there is nothing to replace it?

Applied the alternative way of doing this.

johnynek · 2017-05-08T20:42:08Z

storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisSetStore.scala

    kv match {
      case (k, Some(v)) =>
-        client.del(Seq(k)) // put, not merge, semantics
-        ttl.map(exp => client.expire(k, exp.inSeconds))
+        client.dels(Seq(k)) // put, not merge, semantics


we don't need to wrap with a Seq. Could we just call client.del(k) maybe? Also, this seems like a a race to me. the del could happen after the set. We should use flatMap to sequence:

for { _ <- client.del(k) _ <- ttl.fold(Future.Unit) { exp => client.expire(k, exp.inSeconds) } _ <- set(k, v.toList) } yield ()

del is deprecated unfortunately. Taken care of the race.

johnynek · 2017-05-08T20:42:20Z

storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisSetStore.scala

        set(k, v.toList)
      case (k, None) =>
-        client.del(Seq(k)).unit
+        client.dels(Seq(k)).unit


is there noclient.del(k) we can call?

del is deprecated:
@deprecated("remove netty3 types from public API", "2016-03-24")
def del(keys: Seq[ChannelBuffer]): Future[JLong] =

johnynek · 2017-05-08T20:44:07Z

storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisSortedSetStore.scala

-  override def get(k: ChannelBuffer): Future[Option[Seq[(ChannelBuffer, Double)]]] =
-    client.zRange(k, 0, -1, true).map(
+  override def get(k: Buf): Future[Option[Seq[(Buf, Double)]]] =
+    client.zRange(k, 0.toLong, -1.toLong, true).map(


0L and -1L is the better way to do this.

johnynek · 2017-05-08T20:44:20Z

storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisSortedSetStore.scala

    kv match {
      case (set, Some(scorings)) =>
-        client.del(Seq(set)).flatMap { _ =>
+        client.dels(Seq(set)).flatMap { _ =>


is there no client.del(set)?

There is but it's marked deprecated.

johnynek · 2017-05-08T20:44:33Z

storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisSortedSetStore.scala

          Future.collect(members.multiPut(scorings.map {
            case (member, score) => ((set, member), Some(score))
          }.toMap).values.toSeq).unit
        }
      case (set, None) =>
-        client.del(Seq(set)).unit
+        client.dels(Seq(set)).unit


same comment about client.del(set).

del is deprecated.

johnynek · 2017-05-08T20:45:33Z

storehaus-redis/src/main/scala/com/twitter/storehaus/redis/RedisStore.scala

+  }
+}
+
+object ChannelBuffer2BufInjection extends Injection[ChannelBuffer, Buf] {


this should be a bijection, not an Injection (there is an implicit from Bijection to Injection, by the way so you can pass this as the injection above).

finagle-http-compat has been removed in more recent versions, taking care of that may need non-trivial changes. Will upgrade finagle version in a follow up change.

…, again due to property declaration behavior change in scalacheck. Fix that.

…ase for 2.12

codecov-io · 2017-05-09T05:36:42Z

Codecov Report

Merging #346 into develop will decrease coverage by 0.14%.
The diff coverage is 23.8%.

@@             Coverage Diff             @@
##           develop     #346      +/-   ##
===========================================
- Coverage    61.48%   61.33%   -0.15%     
===========================================
  Files          119      119              
  Lines         1807     1813       +6     
  Branches       118      117       -1     
===========================================
+ Hits          1111     1112       +1     
- Misses         696      701       +5

Impacted Files	Coverage Δ
...scala/com/twitter/storehaus/mysql/MySqlStore.scala	`96.84% <ø> (ø)`	⬆️
...cala/com/twitter/storehaus/mysql/ValueMapper.scala	`54.76% <ø> (ø)`	⬆️
...a/com/twitter/storehaus/mysql/MySqlLongStore.scala	`50% <ø> (ø)`	⬆️
...la/com/twitter/storehaus/redis/RedisSetStore.scala	`0% <0%> (ø)`	⬆️
...a/com/twitter/storehaus/redis/RedisHashStore.scala	`0% <0%> (ø)`	⬆️
...com/twitter/storehaus/memcache/MemcacheStore.scala	`39.28% <0%> (-3.03%)`	⬇️
...a/com/twitter/storehaus/redis/RedisLongStore.scala	`75% <100%> (ø)`	⬆️
...scala/com/twitter/storehaus/redis/RedisStore.scala	`33.33% <20%> (-5.56%)`	⬇️
.../twitter/storehaus/redis/RedisSortedSetStore.scala	`73.8% <75%> (ø)`	⬆️
.../twitter/storehaus/mysql/MergeableMySqlStore.scala	`91.3% <0%> (+4.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b02d72c...67728c4. Read the comment docs.

oscar-stripe · 2017-05-10T17:47:39Z

+1

After we publish a new version, we should bump the minor version, and remove all the exclusions for mima.

Thanks for doing this.

Ben McNiel and others added 2 commits April 14, 2017 00:32

Fix mysql tests that break with scalacheck 1.13.4

7e4ac53

pankajroark requested review from johnynek and ttim May 8, 2017 18:03

Update finagle and twitter/util versions.

8e39dd2

ttim approved these changes May 8, 2017

View reviewed changes

johnynek reviewed May 8, 2017

View reviewed changes

pankajroark added 5 commits May 8, 2017 20:05

Take care of review comments. Going back to finagle 6.41.

d445f0b

finagle-http-compat has been removed in more recent versions, taking care of that may need non-trivial changes. Will upgrade finagle version in a follow up change.

Use jdk8 for scala2.11 travis.

a8cc7cd

Mongodb tests were going OOM due to creation of client numerous times…

0c739f4

…, again due to property declaration behavior change in scalacheck. Fix that.

Remove mima checks for 2.12 for this change as this is the first rele…

164a3b1

…ase for 2.12

Add excludes for mima failures.

67728c4

pankajroark merged commit 6b130e0 into develop May 10, 2017

pankajroark mentioned this pull request May 12, 2017

Release 0.15.0 prep. Update version in preparation for rc release. #348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scala 2.12 upgrade preview with mysql tests fixed #346

Scala 2.12 upgrade preview with mysql tests fixed #346

pankajroark commented May 7, 2017

pankajroark commented May 7, 2017

ttim left a comment

ttim May 8, 2017

pankajroark May 9, 2017

ttim May 8, 2017

johnynek May 8, 2017

ttim May 8, 2017

johnynek May 8, 2017

pankajroark May 9, 2017

johnynek May 8, 2017

pankajroark May 9, 2017

johnynek May 8, 2017

johnynek May 8, 2017

johnynek May 8, 2017

pankajroark May 9, 2017

johnynek May 8, 2017

pankajroark May 9, 2017

johnynek May 8, 2017

pankajroark May 9, 2017

johnynek May 8, 2017

johnynek May 8, 2017

pankajroark May 9, 2017

johnynek May 8, 2017

pankajroark May 9, 2017

johnynek May 8, 2017

codecov-io commented May 9, 2017 •

edited

Loading

oscar-stripe commented May 10, 2017

Scala 2.12 upgrade preview with mysql tests fixed #346

Scala 2.12 upgrade preview with mysql tests fixed #346

Conversation

pankajroark commented May 7, 2017

pankajroark commented May 7, 2017

ttim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 9, 2017 • edited Loading

Codecov Report

oscar-stripe commented May 10, 2017

codecov-io commented May 9, 2017 •

edited

Loading